Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

timing: Always split histogram with zero as a boundary #436

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

timing: Always split histogram with zero as a boundary #436

wants to merge 1 commit into from

Conversation

smunaut
Copy link
Contributor

@smunaut smunaut commented Apr 29, 2020

If the timing histogram has both positive and negative numbers,
then we ensure that zero is used as a boundary and that at least
25% of the bins are dedicated to both positive and negative side
so you get a decent level of details on both sides.

Obviously that means bin size is different for positive / negative
sides.

Signed-off-by: Sylvain Munaut [email protected]

@daveshah1 daveshah1 requested a review from eddiehung May 1, 2020 06:59
@eddiehung
Copy link
Collaborator

eddiehung commented May 1, 2020 •
edited
Loading

In principle, I think this is a good idea. Can you provide an example output?

Obviously that means bin size is different for positive / negative sides.

I'm not sure about this though, as it breaks the understanding that the area of each bar (excepting perhaps the two ends) are proportional to their frequency. Perhaps it is better to have two historgrams instead?

@smunaut
Copy link
Contributor Author

smunaut commented May 1, 2020

Example output :

The * and + still mean the same of course, but the interval size in picosecond is different between the pos and neg bins.

And yeah, possibly two distinct histograms would make more sense this would also provide more granularity ...

Info: Slack histogram:
Info:  legend: * represents 24 endpoint(s)
Info:          + represents [1,24) endpoint(s)
Info: [ -2275,  -1820) |+
Info: [ -1820,  -1365) | 
Info: [ -1365,   -910) | 
Info: [  -910,   -455) | 
Info: [  -455,      0) |+
Info: [     0,   5216) |************************+
Info: [  5216,  10432) |*****+
Info: [ 10432,  15648) |********************+
Info: [ 15648,  20864) |***********************************+
Info: [ 20864,  26080) |************************************************************ 
Info: [ 26080,  31296) | 
Info: [ 31296,  36512) | 
Info: [ 36512,  41728) | 
Info: [ 41728,  46944) | 
Info: [ 46944,  52160) | 
Info: [ 52160,  57376) | 
Info: [ 57376,  62592) | 
Info: [ 62592,  67808) | 
Info: [ 67808,  73024) | 
Info: [ 73024,  78240) |+

@smunaut
timing: Always split histogram with zero as a boundary
e53680b 
If the timing histogram has both positive and negative numbers,
then we ensure that zero is used as a boundary and that at least
25% of the bins are dedicated to both positive and negative side
so you get a decent level of details on both sides.

Obviously that means bin size is different for positive / negative
sides.

Signed-off-by: Sylvain Munaut <[email protected]>
@smunaut
Copy link
Contributor Author

smunaut commented May 16, 2020

Just added a slightly different version that also prints exactly how many EPs are in there (in addition to the bar). I find it useful to check when I actually except a certain number of EP to fail to make sure only that # and not more are failing.

Info: Slack histogram:
Info:  legend: * represents 1 endpoint(s)
Info:          + represents [1,1) endpoint(s)
Info: [ -1390,  -1112) |      1 | * 
Info: [ -1112,   -834) |     10 | ********** 
Info: [  -834,   -556) |      2 | ** 
Info: [  -556,   -278) |      7 | ******* 
Info: [  -278,      0) |      0 |  
Info: [     0,    452) |      2 | ** 
Info: [   452,    904) |      1 | * 
Info: [   904,   1356) |      3 | *** 
Info: [  1356,   1808) |      5 | ***** 
Info: [  1808,   2260) |      2 | ** 
Info: [  2260,   2712) |      7 | ******* 
Info: [  2712,   3164) |     23 | *********************** 
Info: [  3164,   3616) |      8 | ******** 
Info: [  3616,   4068) |     10 | ********** 
Info: [  4068,   4520) |      6 | ****** 
Info: [  4520,   4972) |      6 | ****** 
Info: [  4972,   5424) |      5 | ***** 
Info: [  5424,   5876) |     56 | ******************************************************** 
Info: [  5876,   6328) |     21 | ********************* 
Info: [  6328,   6780) |      6 | ******

@smunaut
Copy link
Contributor Author

smunaut commented Jun 24, 2020

@eddiehung feedback ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddiehung eddiehung Awaiting requested review from eddiehung

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@smunaut @eddiehung